Conversation
cap10morgan
force-pushed
the
chore/unit-tests-third-times-a-charm
branch
from
bb277e9 to
635fd83
Compare
|
Would love to hear if anyone has any insight into this integration test failure: https://github.com/HarperFast/harper/actions/runs/20474799099/job/58837312244?pr=66 |
|
Do you have more of an explanation for the strategies here?
|
I need to reassess this and write up what I'm seeing. More to come...
There's a ton of flexibility here we could take more / different advantage of, but currently yes, it is a single global switch. We could add more import subpath patterns that simply point at
It migrates the component unit tests, but it's perhaps a little hard to see since git views it as a renaming of the previously-merged component unit tests. And yes, additional migrations will involve similar changes as this PR demonstrates. I plan to migrate one or two more categories of tests and then write up a migration guide for others to use to migrate more. |
...with subpath import patterns
These don't all pass yet, but nice to have to find places we need to fix up.
cap10morgan
force-pushed
the
chore/unit-tests-third-times-a-charm
branch
from
b0a57a2 to
75dc592
Compare
OK it looks like this is unnecessary now. Which is great! So this PR now excludes the commit that changed all of the source code requires / imports to use subpath import patterns. We still can (and IMO should) use them any/everywhere, but they now appear to be out of scope for this PR. Whatever I was running into before that was improved by changing them all seems to no longer be an issue. |
It went away! (🎉) |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
What does the process look like for migrating tests? How much time does it take, or will take, to copy over tests? Is there a script or does AI heavily speed up this process? Will most over of tests copy over with this procedure? Or some?
| reset(); | ||
| }); | ||
| const { describe, it, afterEach } = require('mocha'); | ||
| const assert = require('node:assert/strict'); |
There was a problem hiding this comment.
So this is directly recopying the test from /harperdb, eliminating any .ts conversion or conversion to ESM, right?
There was a problem hiding this comment.
This was added because it is nice to explicitly require mocha and import the test functions? But is not strictly necessary for migration?
There was a problem hiding this comment.
Correct, and because sometimes my IDE realizes these are globals, and sometimes it doesn't. And because I'm allergic to globals. 😝
| assert.equal(status.message, 'Connection lost'); | ||
| }); | ||
|
|
||
| it('should report healthy status', function () { |
There was a problem hiding this comment.
This is also code cleanup? But not necessary, and we have to be careful not to do this if this is accessed (setting this.timeout for example, or using this to hold some state)?
There was a problem hiding this comment.
Yes, and an attempt to make the style more consistent. But yes, if the test needs access to this, then it has to use function () syntax. I've had good luck with eliminating those accesses so far, though. :)
| const UNIT_TEST_DIR = __dirname; | ||
| const ENV_DIR_NAME = 'testEnv'; | ||
| const ENV_DIR_PATH = path.join(UNIT_TEST_DIR, ENV_DIR_NAME); | ||
|
|
There was a problem hiding this comment.
It looks like we haven't copied some functions, like cleanUpDirectories that are in use by other tests that will be migrated? But we have a plan to replace?
There was a problem hiding this comment.
Yeah I only wanted to migrate fns that are actually used by tests in this repo. So if there are other testUtils fns needed by tests we migrate down the road, I think we should bring in those fns in the corresponding migration PRs.
All good questions, but I don't think we'll know the answers with too much certainty until we migrate a few more. If I had to make an educated guess, I would say the more recently written tests might take part of a day to migrate (an entire dir / category of), and the older tests will take significantly longer / essentially need to be rewritten. But the basic starting point is to copy a directory of tests from harperdb, update the requires to use the import subpath patterns, and then try running them. I have commonly run into sandboxing issues at that point with earlier test migrations (where the tests cause / rely on leakage into / out of the But of course things could always be different on this branch. Hopefully in ways that I can borrow fixes from previous attempts. |
|
It seems like "migrate" keeps encompassing an effort to change code (globals -> require, function -> arrow function, etc.) prior to bringing it into harper and merging it. This seems contradictory to our goals and values of trying to do as much in the open and public as possible. Why do we need to do this before merging and obscuring these changes from the public? And don't we benefit from tracking any changes to tests with clear issue tracking as well? (why not have issues for changing functions, globals, etc.?)
Why will these need to be rewritten (in the initial migration)? I thought that by using the dist directory, we should be able to modify module records and preserve things like rewrite and stubbing? |
There's always a balance between the core goal and opportunistic, quick, easy cleanups you find along the way. Ideally these "side quest" changes are things that are non-controversial improvements that don't distract from the main goal and are typically low-priority changes that have a hard time getting addressed as a primary goal. If / when they stray too far from that ideal for whatever reason, I'm happy to revert them and save them for a future PR.
I'm just speculating. Perhaps they won't need that! |
Yes, but it has been almost 3 months since we opened started this repo, and I think we have less than 10% of unit tests migrated, so it feels disingenuous to me to be calling any of this "quick"/"easy". Currently any focused development on this repo is significantly hindered and endangered by the lack of unit tests, and at this point seems pretty hard to justify unit test cleanup getting in the way of simply getting the tests in place and running.
I am skeptical that before committing to this repo it is "along the way", but once committed, it really is out of the way.
It seems like focusing on getting as many tests migrated and identifying issues with that should be the priority. |
A lot of this was done much earlier in the process. And like I said, I'm happy to revert any / all of it. We're on the same page w/r/t priorities.
Not sure I follow, but like I said, I'm happy to revert any / all of those changes.
I'm certainly not proposing we delete any tests. We're on the same page regarding priorities. |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Yes. I really wasn't trying to get you to revert things either, honestly all your work looks great, and I appreciate it. I just didn't want to setup barriers to further migration. This certainly seems like it establishes a pattern we could follow easily for more migrations (and I assume that will be in subsequent PRs, so happy to merge this and continue with that).
Great! Once this lands, I'll open PRs for some of the test categories I migrated previously and so hopefully we'll have quite a few unit tests in short order. Then I can write up a migration guide and we can start trying to migrate others. Might make sense at that point to write up GitHub issues for the migration of test categories (roughly |
|
That sounds great, thank you! |
|
I don't understand why this code was changed from TypeScript to JavaScript and ESM to CJS. This baffles me. |
We discussed this in previous PRs and engineering meetings. We decided to redo the unit test migration and just keep the unit tests as CJS for now. So this change reflects that decision. |
I don't recall this being mentioned in any previous PRs or engineering meetings, otherwise I wouldn't ask. I don't understand the rationale for keeping unit tests as CJS for now. This is obviously making more work for you/us. |
One example: #15 ...but the bulk of the conversation was in a Zoom meeting around then. |
Thank you for the link @cap10morgan ! Unfortunately, I do not agree with the direction of this PR. I fail to understand the reason (technical or otherwise) for abandoning TypeScript and ESM. I cannot green light this PR. Whomever made the call to abandon TS and ESM should be reviewing this PR. |
This PR isn't really about these unit test themselves, but about the pattern we are setting up for migrating unit tests from /harperdb. And those tests are (predominantly) CJS (these tests originate here: https://github.com/HarperFast/harperdb/tree/main/unitTests/components). That's just the fact of reality. So the change is to establish a pattern that minimizes the changes needed when we migrate/copy the tests over, so we can do that as efficiently as possible and then any changes that we want to make to unit tests are actually properly tracked in version control/commits (and has associated tickets, hopefully). So the really is not about making or precluding any decisions about eventually migrating tests to ESM, TS, vitest, or anything else, those are still very much open possibilities. But this is about preserving our core values, that we want to do that openly, in the public, with a clear commit history. And again, the immediate priority is to get the tests migrated and working; which is actually critical for effectively and sanely modifying tests to another form, so that we can start with working/passing unit tests and then modify tests with confidence that the code itself works and any changes to unit tests can be isolated and fixed to pass unit tests. (migrating and modifying tests at the same time is asking for trouble of not know if failures are due to source code or unit test changes). (we could keep a branch of the changes here from the .ts -> .ts with the new module specifiers, but its not that many tests, and I assume it is preserved in the commit history anyway, for future usage). |
|
My $.02, fwiw, why don't do do both, more or less (but probably not all at once)? Pull over all of the existing unit tests into, say, test-legacy/unit/ and have clean, modern, unit tests in test/unit/ (or whatever path patterns that make sense)? Get the value in all of our existing tests and make it clear and easy to build new tests and migrate existing while having at least similar coverage to what we have now? There's maybe little harm to have the extra/duplicate coverage in the meantime? Separate branch(es) might be more difficult to coordinate and maintain - but maybe this proposal would be a real mess to to have in place... 🤔 |
This is an interesting idea, but what is the criteria for what goes in Of all of these possibilities, separating out tests that mutate module records (use rewrite/stubs) actually sounds the most compelling to me, since this is the most significant issue we know of that precludes upgrading some tests to ESM/TypeStrip. I think it is our belief that generally tests that do this will be quite difficult to upgrade to TypeStrip/ESM and tests that don't will likely be relatively easy to switch. Using folders to designate tests that use module record modification (and require more extensive rewriting), would probably be helpful separate designation from tests that need a CJS->ESM upgrade, which could be designated by file extension. And it is likely that tests will be upgraded in multiple phases, so having multiple designations is probably wiser. If we go this route, I don't think there is anything to change in this PR, these tests are already considered "modern" from the module record immutability perspective. And I do recognize that this PR has become a bit confusing; again I think it should be understood as the pattern for how we will do initial copy/migration of tests from /harperdb, but this PR doesn't actually copy/migrate any tests from /harperdb, its diff is a modification of existing tests to show how other tests will be migrating (which isn't the clearest way to demonstrate that). And to that point, merging this PR, at least the test changes, isn't actually necessary for us to get to work on migrating other tests, AFAICT, the main key thing to merge (for other tests to follow this pattern) is the package.json changes to setup the subpath importing. And I suppose it is even feasible to copy tests and subsequently change the paths. I'd be happy to create a separate PR for the package.json changes, if that helps. |
|
Yeah, I think it's time to merge this and iterate forward. Some unit tests are better than no unit tests. This represents a pretty minimal change (in the grand scheme of things; the git diff of this PR actually shows more churn than there really is; this PR undoes some of the premature changes that previously landed in this repo) and is the result of much discussion, research, and pondering how best to proceed. And like Kris said, we haven't fully defined how we want unit tests to evolve in the future. But merging this allows more work to start moving forward and with greater confidence w/ automated tests, IMO. |
4 participants
Supersedes #31. Hopefully third time's a charm!
This is still slightly a work in progress. I need to verify a few more things before merging, but it's coming together nicely and wanted to get it up for review and for my own re-orientation with it after the holiday break.Should be ready for review now!